-
Notifications
You must be signed in to change notification settings - Fork 7
ICRC-123: ledger blocks for recording management actions -- freezing and unfreezing of accounts and principals #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
ICRCs/ICRC-123/ICRC-123.md:3
- [nitpick] Consider rewording to 'account freezing and unfreezing events' for clarity and consistency.
ICRC-123 introduces new block types for recording account and unfreezing events in ICRC-compliant ledgers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
ICRCs/ICRC-123/ICRC-123.md:3
- [nitpick] Consider rephrasing 'recording account and unfreezing events' to 'recording account freeze and unfreeze events' for clarity and consistency with the rest of the document.
ICRC-123 introduces new block types for recording account and unfreezing events in ICRC-compliant ledgers.
ICRCs/ICRC-123/ICRC-123.md
Outdated
|
|
||
| ### Ledger Enforcement Rules | ||
|
|
||
| - A ledger **MUST reject** any transfer transaction (`icrc1_transfer` or `icrc2_transfer_from`) where the **sender or recipient account is currently RESTRICTED**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the spender of an icrc2_transfer_from transaction?
Can/should we say anything about the errors that should be returned in this case? TransferError in ICRC-1 and TransferFromError in ICRC-2 don't seem to specify any variants that would be a good fit here - can we recommend some specific GenericError error code and/or message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first question: thanks for bringing it up. The current version should preclude it.
For the second: I wouldn't introduce a code right now but a reasonable message ("Sending account/receiving account is frozen") may be good. Open to suggestions.
ICRCs/ICRC-123/ICRC-123.md
Outdated
|
|
||
| ### Querying Freeze Status | ||
|
|
||
| Ledgers implementing this standard SHOULD expose a query interface (e.g., `is_account_restricted(account): bool`) that returns whether an account is currently RESTRICTED according to the rules defined in "Account Status". This serves as a convenience layer and does not replace auditing based on block history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we clearly define the query interface to be used? what's the benefit of leaving this up to the implementer?
I think the ecosystem would benefit of having query interfaces clearly defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we could leave it up to the implementer to decide whether to expose such function or not. but if they decide to implement it, it would be good to have a common/reusable function name for that.
clients might need to implement some error handling though if this function is not present. (or we add an additional query function to check whether this is function is available or not 🤣 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this choice is in line with the idea that in this standard we only define the block formats and a standarised interface can then go into a second standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in any way it would be good to be consistent on this matter also for ICRC-124 where querying the ledger status is not mentioned specifically at this point.
maybe we should mention again that the interface definition is just a suggestion and might be standardized in another ICRC
| vec { | ||
| // ... other supported types like ICRC-1 ... | ||
| variant { Record = vec { | ||
| record { "btype"; variant { Text = "123freezeaccount" }}; | ||
| record { "url"; variant { Text = "[https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md](https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md)" }}; // Placeholder URL | ||
| }}; | ||
| variant { Record = vec { | ||
| record { "btype"; variant { Text = "123unfreezeaccount" }}; | ||
| record { "url"; variant { Text = "[https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md](https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md)" }}; // Placeholder URL | ||
| }}; | ||
| variant { Record = vec { | ||
| record { "btype"; variant { Text = "123freezeprincipal" }}; | ||
| record { "url"; variant { Text = "[https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md](https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md)" }}; // Placeholder URL | ||
| }}; | ||
| variant { Record = vec { | ||
| record { "btype"; variant { Text = "123unfreezeprincipal" }}; | ||
| record { "url"; variant { Text = "[https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md](https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-123.md)" }}; // Placeholder URL | ||
| }}; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove placeholder URL comment and check the url content (currently a markdown link)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
ICRCs/ICRC-123/ICRC-123.md
Outdated
|
|
||
| This "latest-action-wins" rule implies: | ||
| - A freeze of an account (`123freezeaccount`) can be lifted by a later unfreeze of the same account (`123unfreezeaccount`) or by a later unfreeze of the owning principal (`123unfreezeprincipal`). | ||
| - A freeze of a principal (`123freezeprincipal`) can be lifted by a later unfreeze of that principal (`123unfreezeprincipal`). It also implicitly unfreezes all accounts owned by that principal unless a more recent, specific `123freezeaccount` block targets one of those accounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - A freeze of a principal (`123freezeprincipal`) can be lifted by a later unfreeze of that principal (`123unfreezeprincipal`). It also implicitly unfreezes all accounts owned by that principal unless a more recent, specific `123freezeaccount` block targets one of those accounts. | |
| - A freeze of a principal (`123freezeprincipal`) can be lifted by a later unfreeze of that principal (`123unfreezeprincipal`). It also implicitly unfreezes all accounts owned by that principal unless a more recent, specific `123freezeaccount` block targets one of those accounts. Furthermore a freeze of specific accounts of this principal can be lifted by a later `123unfreezeaccount`. |
| - A freeze of an account (`123freezeaccount`) can be lifted by a later unfreeze of the same account (`123unfreezeaccount`) or by a later unfreeze of the owning principal (`123unfreezeprincipal`). | ||
| - A freeze of a principal (`123freezeprincipal`) can be lifted by a later unfreeze of that principal (`123unfreezeprincipal`). It also implicitly unfreezes all accounts owned by that principal unless a more recent, specific `123freezeaccount` block targets one of those accounts. | ||
|
|
||
| ### Ledger Enforcement Rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rules for ICRC122 methods are missing
ICRCs/ICRC-123/ICRC-123.md
Outdated
| - A ledger **MUST reject** any `icrc1_transfer` or `icrc2_transfer_from` transaction where the **sender** account (the `from` account in the operation) is currently RESTRICTED. | ||
| - The ledger **MAY**, according to its policy, also reject `icrc1_transfer` or `icrc2_transfer_from` transactions if the **recipient** account (the `to` account in the operation) is RESTRICTED, or it MAY allow incoming funds to a RESTRICTED recipient. | ||
| - **ICRC-2 Operations:** | ||
| - **`icrc2_approve` (Granting Approval):** If an account is RESTRICTED, its owner **MUST NOT** be able to authorize an `icrc2_approve` transaction where this restricted account is the one granting the approval (i.e., the `account` argument in `icrc2_approve` which specifies the owner of the funds being approved for spending). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **`icrc2_approve` (Granting Approval):** If an account is RESTRICTED, its owner **MUST NOT** be able to authorize an `icrc2_approve` transaction where this restricted account is the one granting the approval (i.e., the `account` argument in `icrc2_approve` which specifies the owner of the funds being approved for spending). | |
| - **`icrc2_approve` (Granting Approval):** If an account is RESTRICTED, its owner **MUST NOT** be able to authorize an `icrc2_approve` transaction where this restricted account is the one granting the approval (i.e., the `caller` in combination with the `from_subaccount` argument in `icrc2_approve` which together specify the `from` account that is being approved for spending). |
Real World Asset ledgers require several management APIs that would allow authorised entities to carry out ledger changes. This standard introduces four new blocktypes:
123freezeaccountthat records that a specific account was frozen by an authorised party123unfreezeaccountthat records that a specific account was unfrozen by an authorised party123freezeprincipalthat records that all of the accounts of a specific principal were frozen by an authorised party123unfreezeprincipalthat records that all of the accounts of a specific principal were unfrozen by an authorised party